[PM-28855] Update credential identities store on cipher changes on iOS extensions #2169
Conversation
…tities store when cipher get CUD on iOS extensions.
|
Claude finished @fedemkr's task —— View job Code Review CompleteSummaryThis PR introduces an efficient mechanism to update the OS credential identities store on iOS 17+ extensions by subscribing to individual cipher changes rather than batch-replacing all credentials. The implementation is architecturally sound with strong test coverage (94.9%). Key Changes:
FindingsFinding 1: Missing error handling for
|
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2169 +/- ##
==========================================
+ Coverage 85.39% 85.42% +0.03%
==========================================
Files 1731 1732 +1
Lines 145718 146209 +491
==========================================
+ Hits 124434 124899 +465
- Misses 21284 21310 +26 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| private func subscribeToCipherChanges() { | ||
| cipherChangesSubscriptionTask?.cancel() | ||
| cipherChangesSubscriptionTask = Task { [weak self] in | ||
| guard let self else { return } | ||
|
|
||
| do { | ||
| for await cipherChange in try await cipherService.cipherChangesPublisher().values { |
There was a problem hiding this comment.
❌ Critical: If cipherChangesPublisher() throws during initialization, the error is logged but the subscription task completes silently. This means cipher changes won't update the credential store, breaking autofill functionality without any visible indication.
Recommendation: Add retry logic or fail-safe handling:
private func subscribeToCipherChanges() {
cipherChangesSubscriptionTask?.cancel()
cipherChangesSubscriptionTask = Task { [weak self] in
guard let self else { return }
var retryCount = 0
let maxRetries = 3
while retryCount < maxRetries && !Task.isCancelled {
do {
let publisher = try await cipherService.cipherChangesPublisher()
for await cipherChange in publisher.values {
switch cipherChange {
case let .inserted(cipher):
await upsertCredentialsInStore(for: cipher)
case let .updated(cipher):
await upsertCredentialsInStore(for: cipher)
case let .deleted(cipher):
await removeCredentialsInStore(for: cipher)
}
}
break // Success
} catch {
errorReporter.log(error: error)
retryCount += 1
if retryCount < maxRetries {
try? await Task.sleep(nanoseconds: 1_000_000_000)
}
}
}
}
}
| let fido2Identities = try await clientService.platform().fido2() | ||
| .authenticator( | ||
| userInterface: fido2UserInterfaceHelper, | ||
| credentialStore: fido2CredentialStore, | ||
| ) | ||
| .credentialsForAutofill() |
There was a problem hiding this comment.
💭 Memory Concern: Does credentialsForAutofill() load all Fido2 credentials into memory before the .filter operation? If so, this defeats the memory efficiency goal of this PR for large vaults.
Question: Can we fetch Fido2 credentials for a specific cipher ID directly from the SDK, or is credentialsForAutofill() already efficient/lazy?
| guard let cipherData = object as? CipherData, | ||
| cipherData.userId == userId, | ||
| let cipher = try? Cipher(cipherData: cipherData) else { continue } |
There was a problem hiding this comment.
Cipher(cipherData:) throws (corrupted data), the change event is dropped without logging, potentially causing credential store to become out of sync.
Recommendation: Log failures:
guard let cipherData = object as? CipherData,
cipherData.userId == userId else { continue }
guard let cipher = try? Cipher(cipherData: cipherData) else {
Logger.application.error("Failed to decode cipher \(cipherData.id ?? "unknown") for change notification")
continue
}
_ = subscriber.receive(.inserted(cipher))
🎟️ Tracking
PM-28855
📔 Objective
Add cipher changes subscription to update credential identities store on iOS extensions.
This differs from the current approach where all ciphers/credentials get updated to be done on individual cipher CUD helping memory efficiency.
As you can see the new
CipherChangePublisherpublishes only when "one" cipher operation was done, but it doesn't when there's a batch operation like replacing all ciphers to avoid memory problems on the extensions.Important
This is a solution for iOS 17+ versions as they have incremental updates on the credential identities store. I didn't add the fallback as it'd imply replacing all identities from all ciphers which means loading and decrypting all (or most) ciphers which may crash on large vaults or have the extension use more memory leaving less for other operations like searching before crashing. The user can always force a sync on the main app to update the OS store and have autofill working for that specific credential again.
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes